buffer: refactor buffer exports #13807

jasnell · 2017-06-20T01:17:42Z

Refactor buffer exports to use module.exports = {} pattern
~~Migrate most errors to use internal/errors~~ _{refack: crossed off, as this was done in errors, buffer: Migrate buffer errors to use internal/errors #13976}

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

buffer

mscdex · 2017-06-20T03:33:27Z

lib/buffer.js

Trott · 2017-06-20T06:08:46Z

lib/buffer.js currently has 100% code coverage in our test suite. If not too onerous, it would be nice to run the coverage report on this PR to make sure the refactoring doesn't introduce code paths that are now untested.

TimothyGu · 2017-06-20T02:03:22Z

lib/buffer.js

I'd prefer using buf1 instead of a in the code as well.

TimothyGu · 2017-06-20T02:06:05Z

lib/internal/buffer.js

Would { FastBuffer: undefined } make it more self-documenting?

TimothyGu · 2017-06-20T02:06:30Z

lib/internal/errors.js

Alphabetical order.

jasnell · 2017-06-20T14:36:13Z

Forgot to mark this as in progress as I still need to add the documentation updates for the new error codes...

BridgeAR · 2017-06-20T21:00:43Z

lib/internal/errors.js

Maybe call the error type something more generic? There could be any variable that may not be negative or that may not be greater than x.

BridgeAR · 2017-06-20T21:03:26Z

lib/internal/errors.js

I am not sure if the error codes should be more generic or not but I think it would make sense if they are. In this case the ERR_INVALID_OPT_VALUE type could also be used as such.

IMHO including the encoding name is better.

BridgeAR · 2017-06-20T21:04:34Z

lib/internal/errors.js

See my comment below. This type is at least redundant to ERR_UNKNOWN_ENCODING but I would use ERR_INVALID_OPT_VALUE instead.

BridgeAR · 2017-06-20T21:07:41Z

lib/internal/errors.js

I'm not sure if these types would only fit for buffers and therefore I recommend to remove the BUFFER part from the name.

There's the whole over specialization of invalid options debate:

try { // do some stuff var f = fs.openSync('g.txt', 'rgh'); // do other stuff catch (e) { if (e.code == 'ERR_INVALID_OPT_VALUE') console.error(e) && process.exit(1) // because it's a programming error else ...; }

Maybe worth creating a new type? OptionError, or have all the codes include a substring?

BridgeAR · 2017-06-20T21:11:08Z

lib/internal/errors.js

This could be consolidated with ERR_ARG_OUT_OF_BOUNDS even though it's not as specific.

refack

Overspecialization of error codes should be discussed

refack · 2017-06-23T23:19:47Z

lib/internal/errors.js

There's the whole over specialization of invalid options debate:

try { // do some stuff var f = fs.openSync('g.txt', 'rgh'); // do other stuff catch (e) { if (e.code == 'ERR_INVALID_OPT_VALUE') console.error(e) && process.exit(1) // because it's a programming error else ...; }

Maybe worth creating a new type? OptionError, or have all the codes include a substring?

jasnell · 2017-06-26T16:35:53Z

Over-generalization of error codes will make those fairly useless and users will end up parsing the error messages again to find out what actually happened. There's little harm in having specific error codes.

refack · 2017-06-26T16:43:35Z

Over-generalization of error codes will make those fairly useless and users will end up parsing the error messages again to find out what actually happened. There's little harm in having specific error codes.

I suggested two solutions: Either create an OptionError type, or use the a prefix ERR_INVALID_OPT_VALUE_XXX.
My main point is that these are predominantly programing errors and less often runtime errors.

refack · 2017-07-15T21:56:43Z

Superseded by #13976

jasnell · 2017-07-17T15:14:37Z

This is not fully superseded. I will refactor. Please do not close.

* Move to more efficient module.exports pattern * Refactor requires * Eliminate circular dependency on internal/buffer * Add names to some functions * Fix circular dependency error in assert.js

jasnell · 2017-07-18T00:41:30Z

Updated. I was waiting for #13976 to land so I could drop the internal/errors commit and refactor this. It should be good to go now.

refack

Would like to see obfuscated whiles replaced with for...

refack · 2017-07-18T14:39:09Z

lib/buffer.js

-  var i = 0;
-  while (++i < byteLength && (mul *= 0x100))
-    val += this[offset + i] * mul;
+    var val = this[offset];


If we're churning let's get some butter: replace with for(...;...;...) to make it more readable. Performance should be on par

I'm going to leave these for now. We can hit those in a separate PR

refack · 2017-07-18T14:43:24Z

So I would like to see the obfuscated whiles replaced with for but that might be outside of this PR intended scope...

refack · 2017-07-18T14:46:52Z

Updated. I was waiting for #13976 to land so I could drop the internal/errors commit and refactor this. It should be good to go now.

@jasnell I'm assuming this is not in progress anymore? Also AFAICT it's semver-patch with the Error changes removed.

jasnell · 2017-07-24T03:49:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9314/

jasnell · 2017-07-24T14:21:03Z

CI is green.

* Move to more efficient module.exports pattern * Refactor requires * Eliminate circular dependency on internal/buffer * Add names to some functions * Fix circular dependency error in assert.js PR-URL: #13807 Reviewed-By: Refael Ackermann <refack@gmail.com>

jasnell · 2017-07-24T14:29:53Z

Landed in 355523f

addaleax · 2017-07-24T17:17:22Z

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know and we’ll add the dont-land-on label.

jasnell · 2017-07-24T17:18:52Z

Unless it causes pain for backporting other things, this is not a priority to backport.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2017

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 20, 2017

jasnell force-pushed the refactor-buffer branch from 8da26a3 to 05db366 Compare June 20, 2017 01:19

mscdex reviewed Jun 20, 2017

View reviewed changes

lib/buffer.js Outdated

Copy link

Contributor

mscdex Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

TimothyGu reviewed Jun 20, 2017

View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label Jun 20, 2017

BridgeAR reviewed Jun 20, 2017

View reviewed changes

refack suggested changes Jun 23, 2017

View reviewed changes

refack closed this Jul 15, 2017

jasnell reopened this Jul 17, 2017

buffer: refactor module.exports, imports

426d42e

* Move to more efficient module.exports pattern * Refactor requires * Eliminate circular dependency on internal/buffer * Add names to some functions * Fix circular dependency error in assert.js

jasnell force-pushed the refactor-buffer branch from 05db366 to 426d42e Compare July 18, 2017 00:40

jasnell changed the title ~~buffer: refactor buffer exports, use internal/errors~~ buffer: refactor buffer exports Jul 18, 2017

refack approved these changes Jul 18, 2017

View reviewed changes

refack removed the wip Issues and PRs that are still a work in progress. label Jul 18, 2017

jasnell removed semver-major PRs that contain breaking changes and should be released in the next major version. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 24, 2017

jasnell closed this Jul 24, 2017

addaleax added dont-land-on-v8.x and removed dont-land-on-v8.x labels Jul 24, 2017

addaleax added dont-land-on-v8.x and removed backport-requested-v8.x labels Jul 24, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Uh oh!

buffer: refactor buffer exports #13807

buffer: refactor buffer exports #13807

Uh oh!

Conversation

jasnell commented Jun 20, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 26, 2017

Uh oh!

refack commented Jun 26, 2017

Uh oh!

refack commented Jul 15, 2017

Uh oh!

jasnell commented Jul 17, 2017

Uh oh!

jasnell commented Jul 18, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 18, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

addaleax commented Jul 24, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jasnell commented Jun 20, 2017 •

edited by refack

Loading

refack left a comment •

edited

Loading

refack commented Jul 18, 2017 •

edited

Loading